Add ignore validation options and static methods for validation of claims independent to token parsing #175

selimok · 2016-09-30T22:05:11Z

This pull request contains the features requested in #80 and additionally code refactoring to reuse claim validation codes in static validation methods.

With these changes we can:

Parse token by ignoring expiration, notbefore claim values or signature problems.
Do these validations explicitly and separately by calling static validation methods.

Use case: As part of our token expiration strategy, we create tokens with short life spans and re-create new tokens if needed. To do this in more convenient way, we need aforementioned ignore methods while parsing. This way we can do additional security checks like XSRF token validation or check JWT integrity by validating signature seperately, even if the JWT token expired.

P.S.: I must add new unit test as Java code, since I have no idea how to write code in groovy. Additionally I made some code refactoring and splitted huge methods into smaller. I hope the names of new methods are appropriate to existing (if any) naming conventions.

Thanks.

…principals

coveralls · 2016-09-30T22:08:20Z

Coverage decreased (-0.6%) to 99.369% when pulling 0f4a6c1 on selimok:master into 48dae36 on jwtk:master.

lhazlewood · 2016-09-30T22:11:53Z

Please write the tests in Groovy - we have standardized on it for testing and it's even easier than Java. We won't accept a PR with Java tests or without 100% coverage.

lhazlewood · 2016-09-30T22:12:25Z

Actually, we can convert the Java to Groovy - I don't want to discourage your PR. If you can though, please try!

selimok · 2016-09-30T22:14:08Z

Ok, i will give a try. Thanks. And sorry for the test coverage :(

lhazlewood · 2016-09-30T22:15:48Z

No worries! We're happy to have the help!

coveralls · 2016-09-30T23:29:18Z

Coverage decreased (-0.6%) to 99.369% when pulling e974307 on selimok:master into 48dae36 on jwtk:master.

…dditional tests

coveralls · 2016-09-30T23:47:46Z

Coverage decreased (-0.09%) to 99.91% when pulling 8ebd92c on selimok:master into 48dae36 on jwtk:master.

coveralls · 2016-10-01T00:00:45Z

Coverage decreased (-0.09%) to 99.91% when pulling 8ae456f on selimok:master into 48dae36 on jwtk:master.

selimok · 2016-10-01T00:05:35Z

Do you have an idea how can fix that https://coveralls.io/jobs/18754160/source_files/1580589642#L15 ?

Thanks.

lhazlewood · 2016-10-01T00:08:46Z

src/main/java/io/jsonwebtoken/JwtParser.java

+     * @return the parser for method chaining.
+     * @see SignatureException
+     */
+    JwtParser ignoreSignature();


I'm not sure we want to allow this - it is not good for a spec-compliant parser to allow avoiding mandated security rules. It's probably better to catch the exception and then ignore it if you want to - otherwise it is not obvious in code that you're ignoring security checks.

Ok we can remove this method. Security comes first. :)

lhazlewood · 2016-10-01T00:09:48Z

src/main/java/io/jsonwebtoken/JwtParts.java

+ *
+ * @since 0.8
+ */
+public class JwtParts {


This is not entirely correct. A JWT can be a JWS or JWE, and they have different numbers of 'parts' (Unsecured JWT = 2 parts, JWS = 3, JWE = 5). Whatever implementation we'd have around that needs to account for any/all 3 of these scenarios, and probably typed accordingly, e.g. JwsParts extends JwtParts and JweParts extends JwtParts.

Because JWE is being implemented in the jwe branch, you might not need to worry about jwe parts yet, but whatever implementation we create should be extensible enough to support that upcoming feature/release.

lhazlewood · 2016-10-01T00:11:01Z

src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java

+			if (!this.ignoreExpiry) {
+	            //https://tools.ietf.org/html/draft-ietf-oauth-json-web-token-30#section-4.1.4
+	            //token MUST NOT be accepted on or after any specified exp time:
+	            ValidationHelper.validateExpiration(header, claims, this.allowedClockSkewMillis, this.clock);


No static/helper methods in the implementation please. Such methods are not object-oriented - they are more procedural and harder to test.

lhazlewood · 2016-10-01T00:13:10Z

src/main/java/io/jsonwebtoken/impl/ValidationHelper.java

+/**
+ * This class contains static methods for validation of JWT claims.
+ */
+public class ValidationHelper {


Please no 'Helper' or 'Utils' or such classes for anything other than things that augment the Java language itself (e.g. Strings.whatever, Objects.whatever, Assert.whatever are fine because they augment the Java language and those things are often not overridable - such as String being final) - these typically often represent procedural (non OO) logic.

If anything, this would be a Validator or something similarly named. Additionally, anything that can be configured/overridable should be an interface and a default implementation provided so that people can plug in different implementations or override them as desired, e.g. Validator and DefaultValidator (assuming it is correct to have such a component).

Upon further review of this class, it appears the most OO/pluggable way for this it to have a generic Validator interface and you could have multiple implementations, e.g. ExpirationValidator PrematureValidator, etc. For example:

public interface Validator<T> { public void validate(T value); }

And the type used by the parser would be Validator<Claims>

Ok i will made some changes and remove that static methods. Thanks for advice.

The problem with Validator interface, that we cannot put additional method parameters into the validate method. For example for checking expires claim we need to optionally provide allowedClockSkewMillis or clock. Do you have any suggestion for this?

lhazlewood · 2016-10-01T00:29:12Z

I added a code review - the use of the static class is why you're getting code coverage failures. HTH!

- Remove ValidationHelper and its static methods. - Create JwsParts class and extend it from JwtParts - Implement validation methods inside of JwtParser according to OO principals. - P.S.: Unit tests follows...

coveralls · 2016-10-03T00:03:34Z

Coverage decreased (-2.5%) to 97.484% when pulling 735934d on selimok:master into 48dae36 on jwtk:master.

coveralls · 2016-10-03T00:49:34Z

Coverage remained the same at 100.0% when pulling 80758c2 on selimok:master into 48dae36 on jwtk:master.

selimok · 2016-10-03T00:51:01Z

Hi @lhazlewood, can you please check my changes again. Thx. :)

coveralls · 2016-10-05T12:34:12Z

Coverage remained the same at 100.0% when pulling a0e4b69 on selimok:master into 48dae36 on jwtk:master.

lhazlewood · 2016-10-05T17:56:16Z

src/main/java/io/jsonwebtoken/JwtParser.java

+     * @param jwt the compact serialized JWT to check
+     * @return {@code true} if the specified JWT compact string is expired, {@code false} otherwise.
+	 */
+	boolean isExpired(String jwt);


I don't believe this or isPremature is necessary - just parse the JWT - if there is an exception, you know it isn't a valid JWT. Thoughts?

Do you mean isPremature or isExpired or both? In our use case we need really first ignore the expiration validation and then separately check if the token expired or not. I don't really know, if there are other cases for isPremature out there. From my point of view, we can kick all ignoreNotBefore, isPremature and validateNotBefore methods entirely. But why not let them there, may be some one need them for a specific use case.

lhazlewood · 2016-10-05T17:56:39Z

src/main/java/io/jsonwebtoken/JwsParts.java

@@ -0,0 +1,32 @@
+package io.jsonwebtoken;


should be in the io.jsonwebtoken.impl package - we don't want JJWT users to be able to use this class directly.

Right. I will change this.

lhazlewood · 2016-10-05T17:58:03Z

src/main/java/io/jsonwebtoken/JwtParser.java

+     * 
+     * @throws ExpiredJwtException if the validation of 'expiration time' ({@code exp}) claim is failed.
+	 */
+	void validateExpiration(String jwt);


There could be a lot of validation scenarios - I don't feel we should represent them as individual methods on the parser. Same with validateNotBefore. What is the desire for these methods?

isExpired method is just a wrapper on validateExpiration. I explained already above in my first comment, why we need isExpired method. So in my case again we can let validateExpiration method private. But again why to remove this method? Both validateExpiration and validateNotBefore methods was in the code already, just there was a part of the huge parse method. I only moved that parts of the code into the smaller methods and made them accessible. So the decision is up to you.

lhazlewood · 2016-10-05T17:58:16Z

src/main/java/io/jsonwebtoken/JwtParts.java

@@ -0,0 +1,52 @@
+package io.jsonwebtoken;


io.jsonwebtoken.impl

coveralls · 2016-10-06T00:08:06Z

Coverage remained the same at 100.0% when pulling e4ae4c5 on selimok:master into 48dae36 on jwtk:master.

selimok · 2016-10-18T07:45:42Z

Any decisions?

selimok · 2016-12-12T13:44:23Z

Are there anything else that I can do for a PR acceptance?

selimok · 2016-12-13T10:38:10Z

If my PR less likely to be accepted, I would open a new feature request for setting clock skew seperately for notBefore and expiry claims. So that I can at least explicitly set a great amount of clock skew to simulate ignore expiry behaviour. Currently if I set the clock skew value, it affects both nb and exp claim validation.

Thanks.

lhazlewood · 2016-12-13T17:34:29Z

Hi @selimok - sorry - we just haven't had time to review this due to conferences (Gartner, re:Invent) and holiday/end-of-quarter deadlines. We'll review this as soon as we can!

selimok · 2016-12-13T22:48:56Z

Hi @lhazlewood, no problem. Sorry for my impatience.
Thanks in advance for your effort.

lhazlewood · 2016-12-14T07:30:27Z

@selimok No worries! I'm sorry for the delay! We really appreciate your patience, and we're grateful for the PR!

torben-amann · 2017-06-08T13:55:59Z

Hey, what's the status of this pr? We are also trying to accomplish a validation without expiration date even though it's contained in the jwt.

pvegh · 2017-06-14T08:33:59Z

Wow @selimok delivers a PR and all the requested changes but 6 months later there is still no feedback from the devs. 6 months!
This is a great example how not to handle an open source project.
If you have no time to support it anymore please add it to the README.MD so that others can decide if they really want to integrate this or look for another solution.

emeraquino · 2018-02-01T14:07:45Z

Any update for the future of this PR?

hudson155 · 2018-06-08T23:19:27Z

Any update on this?

SeanLMcCullough · 2019-08-01T00:43:40Z

Bump, this feature is important for a project I am working on

jonalbertini · 2020-02-17T10:23:26Z

Years after the PR, all the quick fix @selimok done. still not merged... now I bet too much conflicts to merge... contribute they said...

lhazlewood · 2020-02-17T22:25:31Z

@jonalbertini we're taking a different route with a proper Validator interface/builder with chaining methods and such for a future release. This one slipped through the cracks because it was too specialized and not flexible enough for custom validation. We're going with a different approach.

See #474

lhazlewood · 2023-09-16T22:28:08Z

Closing just as a matter of housekeeping in favor of the work that will be done via #474. If you feel otherwise, please let us know and we can discuss.

selimok added 4 commits September 28, 2016 00:17

Code refactoring: Split huge methods into parts according clean code …

1aef6ac

…principals

Small improvements

efe3c63

Add some javadocs and make a small code refactoring

d004330

Add unit tests and fix some problems.

0f4a6c1

Update README.md

e974307

selimok added 2 commits October 1, 2016 01:43

Rewrite unit tests in groovy, remove existing java tests, implement a…

68ba8ff

…dditional tests

Merge branch 'master' of ssh://github.com/sandc-software/jjwt

8ebd92c

Revert to original

8ae456f

lhazlewood requested changes Oct 1, 2016

View reviewed changes

Make some changes according to PR review

735934d

- Remove ValidationHelper and its static methods. - Create JwsParts class and extend it from JwtParts - Implement validation methods inside of JwtParser according to OO principals. - P.S.: Unit tests follows...

Implement unit tests for JwtParser

80758c2

Remove unused interface

a0e4b69

lhazlewood requested changes Oct 5, 2016

View reviewed changes

Move JwsParts and JwtParts classes into impl package

e4ae4c5

laurencenoton mentioned this pull request Feb 12, 2017

Parse without validating signature. #205

Closed

lhazlewood closed this Sep 16, 2023

Add ignore validation options and static methods for validation of claims independent to token parsing #175

Add ignore validation options and static methods for validation of claims independent to token parsing #175

Conversation

selimok commented Sep 30, 2016

coveralls commented Sep 30, 2016

lhazlewood commented Sep 30, 2016

lhazlewood commented Sep 30, 2016

selimok commented Sep 30, 2016

lhazlewood commented Sep 30, 2016

coveralls commented Sep 30, 2016

coveralls commented Sep 30, 2016

coveralls commented Oct 1, 2016

selimok commented Oct 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhazlewood Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhazlewood Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

lhazlewood Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhazlewood commented Oct 1, 2016

coveralls commented Oct 3, 2016

coveralls commented Oct 3, 2016

selimok commented Oct 3, 2016

coveralls commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhazlewood Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 6, 2016

selimok commented Oct 18, 2016

selimok commented Dec 12, 2016

selimok commented Dec 13, 2016

lhazlewood commented Dec 13, 2016

selimok commented Dec 13, 2016

lhazlewood commented Dec 14, 2016

torben-amann commented Jun 8, 2017

pvegh commented Jun 14, 2017

emeraquino commented Feb 1, 2018

hudson155 commented Jun 8, 2018

SeanLMcCullough commented Aug 1, 2019

jonalbertini commented Feb 17, 2020

lhazlewood commented Feb 17, 2020 • edited Loading

lhazlewood commented Sep 16, 2023

lhazlewood Oct 1, 2016 •

edited

Loading

lhazlewood Oct 1, 2016 •

edited

Loading

lhazlewood Oct 1, 2016 •

edited

Loading

lhazlewood Oct 5, 2016 •

edited

Loading

lhazlewood commented Feb 17, 2020 •

edited

Loading